Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPIKE] Add bundled-only "browser" package exports #3920

Closed
wants to merge 3 commits into from

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Jul 6, 2023

This spike adds "pre-bundled" package.json "browser" exports whilst defaulting to separate modules

Whilst not included in this PR, we could resolve UMD for browser-based bundlers alongside CommonJS

(For example, Browserify and webpack v4)

".": {
  "browser": {
    "sass": "./dist/govuk/all.scss",
    "import": "./dist/govuk/all.bundle.mjs",
    "require": "./dist/govuk/all.bundle.js",
    "default": "./dist/govuk/all.bundle.js"
  },
  "sass": "./dist/govuk/all.scss",
  "import": "./dist/govuk/all.mjs",
  "require": "./dist/govuk/all.cjs",
  "default": "./dist/govuk/all.bundle.js"
}

Split out from:

Includes a commit from #3906 to improve our export test output

@colinrotherham colinrotherham requested a review from a team as a code owner July 6, 2023 18:10
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3920 July 6, 2023 18:11 Inactive
@colinrotherham colinrotherham changed the title [SPIKE] Add bundled-only package exports for "browser" condition [SPIKE] Add bundled-only "browser" package exports Jul 6, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3920 July 11, 2023 15:02 Inactive
@romaricpascal
Copy link
Member

Digging around a bit, it seems that neither Browserify1 nor Webpack 42 would use that field. Is this field meant for other tools/versions? Or is it mean to be browser at top level, which both would understand (but double up with main which they already understand).

Footnotes

  1. Judging by its documentation Browserify looks at main and browser at the top level of package.json rather than inside exports

  2. Support for exports in package.json seems to only have landed in Webpack 5 so wouldn't be used by Webpack 4

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Jul 14, 2023

@romaricpascal Sorry I didn't explain very well 😆

Remember we tried compiling separate CommonJS modules?

This PR would let unpkg, webpack 4 and Browserify stay on top-level "browser" as UMD

But modern bundlers get require() or import (with tree-shaking) via package exports as CommonJS, but we'd still offer a route for those applying a "browser" condition to get bundles if needed

We’re likely to add more package export matrix combinations in future

But the flag `--conditions` was only added in Node.js v12.19.0

So rather than do the hard job to “opt out” Node.js 12.18.0 from _every_ incompatible matrix combination, let’s just “opt in” to the package export tests we know will run
We may want to add `[browser]` and `[browser, import]` to differentiate exports for Browserify and webpack v4 in future
Update package exports and add `[browser]` and `[browser, import]` conditions to differentiate exports for Browserify and webpack v4
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3920 July 17, 2023 09:33 Inactive
@romaricpascal
Copy link
Member

None of these tools mention a browser field inside exports for getting their dependency (webpack and Browserify talk about one at the top level of package.json, and Unpkg mentions its own unpkg field). So what I was trying to clarify is whether the configuration was in the right place.

Thinking on it some more, I think until we get a clear demand for automatically linking to an UMD build for these old bundlers, we should avoid adding more complexity to our exports. People can require the relevant path in their code, so wouldn't be blocked. That's also one less path that govuk-frontend may resolve to, which simplifies support.

If demand comes in, though, happy to look into it further. The change wouldn't be breaking anyway as people using the full path wouldn't be forced to make the update.

@colinrotherham
Copy link
Contributor Author

Yeah, don't worry, hope it's clearer now

Definitely, we split this PR out when CommonJS was removed in #3726 (comment)

Won't need looking at again unless we drop UMD as the default for require()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants